Clean up error logging in test #1110
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
size/L
|
/test all |
|
/assign @leiyiz |
| @@ -157,7 +157,7 @@ func testAttachWriteReadDetach(volID string, volName string, instance *remote.In | |||
| testFile := filepath.Join(a.publishDir, "testfile") | |||
| err := testutils.WriteFile(instance, testFile, testFileContents) | |||
| if err != nil { | |||
| return fmt.Errorf("Failed to write file: %v", err) | |||
| return fmt.Errorf("Failed to write file: %v", err.Error()) | |||
There was a problem hiding this comment.
- what is the reason we need
err.Error()here? .Error()returns a string so perhaps%sor%qinstead of%v?
There was a problem hiding this comment.
Keep consistency in logging. Right now we use either %w with err or %v with err.Error() when the error is not nil.
There was a problem hiding this comment.
Sorry, I still don't quite understand what's the context behind the need to add ".Error()" for all err messaging, is there a custom Error() function we're preparing?
There was a problem hiding this comment.
.Error() unwrap and return the error message only, err obj itself printing in %v may contain some other information depending on the error, like error code string etc.
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/retest |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: leiyiz, sunnylovestiramisu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Clean up error logging/unwrapping in test for potential test retries.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: